-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Libqrcodegencpp #182
Add Libqrcodegencpp #182
Conversation
867ac38
to
3781100
Compare
I have a patch downstream in Fedora that adapts the websocket plugin to use the system version of qrcodegencpp. Once this is merged, would it be acceptable to get this upstreamed too? |
Read until the end. No, because your patch rely on installed cmake modules (made by qrcodegen-cmake) which is:
I created a new finder that works for any cases that use qrcodegen.hpp. PatTheMav/cmake-finders#2 which will be added to OBS Studio later. And here is the branch for my changes for obs-websocket: https://github.com/tytan652/obs-websocket/tree/last_ws_dep_standing I discovered the existence of libqrcodegencpp package through your patch and thank you for that. But it does not work in all cases, so I had create something that will work almost anywhere. |
From generated qrcodegen-cpp-targets: # Create imported target qrcodegencpp::qrcodegencpp
add_library(qrcodegencpp::qrcodegencpp SHARED IMPORTED)
...
# Import target "qrcodegencpp::qrcodegencpp" for configuration "Release"
set_property(TARGET qrcodegencpp::qrcodegencpp APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(qrcodegencpp::qrcodegencpp PROPERTIES
IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib64/libqrcodegencpp.so.1.8.0"
IMPORTED_SONAME_RELEASE "libqrcodegencpp.so.1"
) Targets will be generated automatically here: https://github.com/EasyCoding/qrcodegen-cmake/blob/master/CMakeLists.txt#L204-L207 |
|
Sorry again, @xvitaly I can reproduce it, we are in a transition period in our CMake build-system and I modified the wrong file for Linux. I will write a issue on the qrcodegen-cmake repo rather than continuing the off-topic here. |
Great. I'll take a look. |
57d694e
to
a7478d3
Compare
e13f1f7
to
26c6327
Compare
26c6327
to
7b85791
Compare
7b85791
to
4f88130
Compare
Commits:
have been squashed since changes were approved. |
I almost wish that instead of being in a single script, this were two separate scripts, like:
But I'm not sure that would necessarily be any "better" than what is already here, and it might be more convoluted to get that working, so this is fine as-is if @PatTheMav is already fine with it. |
They can't be separated since we can't build |
That's actually an oversight on my end - the CMake files need to be added via a patch file, not via a second checkout (and using relative path patterns like |
I will try to avoid the "relative path", but I will not increase the maintenance cost by generating a patch from a git repo meant to be copied inside another repo. |
That's the canonical way we add CMake support to projects that don't have it. The other option would be a fork that has that patch applied. It seems very roundabout to me to have one repository with the sources, and another repository with the build system files, especially as you cannot combine both into the same directory. |
Unfortunately, the CMake repo itself recommends this method:
So as far as that project is concerned, git cloning both projects is correct. Though, they suggest side-by-side checkouts and not one within the other. The suggested symlink step achieves the same end result as putting both projects into the same directory, but keeps separate git checkout directories. |
https://github.com/nayuki/QR-Code-generator C++ implementation tweaked to be shipped as libraries.
6f069b8
to
7300dfa
Compare
Yeah "one within the other" is not possible (except for downloading the tarball from GitHub and extracting it in place). I don't like it, but the implementation in the PR is fine. |
Right. I think we can explore an alternative implementation in the future, or encourage the base project to add proper CMake support. |
The QR-Code-generator upstream is too unfriendly to external contributors. They don't accept pull requests.
This is a good idea. LibreOffice has already done this and moved to another QR code library.
Our PR with full CMake support: nayuki/QR-Code-generator#140 That's why we created our own repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace nit.
https://github.com/nayuki/QR-Code-generator C++ implementation tweaked to be shipped as libraries.
7300dfa
to
8f6f816
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine. Just need to determine merge order with respect to other pending PRs.
Description
Some Linux distribution (e.g. Ubuntu, Fedora) tweak https://github.com/nayuki/QR-Code-generator to ship it's C and C++ implementations as libraries.
One of the methods is to add files from https://github.com/EasyCoding/qrcodegen-cmake and use CMake to build the libraries which is the method took for this PR.
The C implementation (libqrcodegen) is removed since it is not needed.
I'm aware of #174, so if it get merged first, I will update the pwsh script to add
$Targets
.Motivation and Context
Reduce the number of submodules in obs-studio repo and provides those as libraries in obs-deps.
Also this would allow to make obs-websocket submodule-less.
How Has This Been Tested?
CI test build static version, and tested shared on VMs.
tytan652/obs-studio#13 provides binaries:
Types of changes
Checklist: